Set DisableTargetSecurityGroupAssignment to "true" for new loadbalancers#1239
Set DisableTargetSecurityGroupAssignment to "true" for new loadbalancers#1239nschad wants to merge 5 commits into
Conversation
Signed-off-by: Niclas Schad <niclas.schad@stackit.cloud>
ske-prow
Bot
added
kind/enhancement
…te call Signed-off-by: Niclas Schad <niclas.schad@stackit.cloud>
Signed-off-by: Niclas Schad <niclas.schad@stackit.cloud>
ske-prow
Bot
added
size/M
| } | ||
|
|
||
| // For new lb's always set DisableTargetSecurityGroupAssignment to true | ||
| lb.DisableTargetSecurityGroupAssignment = new(true) |
There was a problem hiding this comment.
This function is not only called for new loadbalancers but also for updating existing ones. If you mention that this is immutable, we should not set it to true by default
There was a problem hiding this comment.
I'm aware that is called by the Update() but the result in that case is not used. This is already tested
Setting it to true by default is literally the Task, lol.
There was a problem hiding this comment.
This will be eventually removed anyway again
There was a problem hiding this comment.
it will not be removed in the future, it will just be made customizeable :)
There was a problem hiding this comment.
after having a chat with the LB guys, we should definitely also set this value in the update + delete call.
They said that their api (sometimes) needs those fields, even though they are immutable.
So in the update call, just use the value that got returned from the get.
Also in the update call when deleting -> use the value that we got from the returned LB.
It is also worth adding a test for that.
We should test that when creating -> always true.
When updating: true or false, depending on the value of the GET LB
Signed-off-by: Niclas Schad <niclas.schad@stackit.cloud>
…nt set to true Signed-off-by: Niclas Schad <niclas.schad@stackit.cloud>
ske-prow
Bot
added
size/L
|
@fischerman: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fischerman The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
4 participants
How to categorize this PR?
/kind enhancement
What this PR does / why we need it:
Pre-requisite for the LB Team to clean-up their workaround in their API.
Which issue(s) this PR fixes:
Fixes #1185
Special notes for your reviewer:
Breaking changes: